-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(logs): add support for deletion protection configuration #36583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
|
I'm trying to run the integration test according to INTEGRATION_TESTS.md to also create the snapshot, but unfortunately don't get it done successfully.
Appreciate any help. |
|
@robert-hanuschke Are you specifying the test file accurately using a relative path from the current directory? |
|
I am in the test directory of the module, the .ts file is there, for the integ call I just replace the ending with .js as I understood the instructions
|
|
@robert-hanuschke you have to build the integ framework first, execute this command at the root of the repo after this is done you should be able to run |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
@dalatrex2 thank you! This was connected to the errors I reported in the initial PR text. First, the lerna command failed simiarly. I now found out that I was on node 25 in my terminal. After setting it to the lts version, the commands succeed. My command outputs look fine - the rosetta script is complaining about the unrelated module aws-amplify-alpha yarn test aws-logs
yarn integ test/aws-logs/test/integ.loggroup-deletionprotection.js --update-on-failed
/bin/bash ./scripts/run-rosetta.sh
yarn test (shortened)
|
|
|
||||||||||||||
|
|
||||||||||||||
ozelalisen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your first contribution, left some minor comments, your implementation is correct, adjusted pr title as this will be shown in the changelog and commit, which should include module that you are adding and descriptive enough
...ges/@aws-cdk-testing/framework-integ/test/aws-logs/test/integ.loggroup-deletionprotection.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
Co-authored-by: Alisen Ozel <74890690+ozelalisen@users.noreply.github.com>
|
Thank you for the review @ozelalisen ! I copied that over as I perceived it to be a pattern, e.g. in |
|
Those tests might have the old patterns, so currently we do not need this. Yes, it is not needed there as well |
ozelalisen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for addressing comments!
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status✅ The pull request has been merged at 6d45573 This pull request spent 29 minutes 41 seconds in the queue, including 29 minutes 32 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue #36554
Closes #36554
This is my very first PR for aws-cdk. Any hints on what I could do better are greatly appreciated.
I am not aware whether I would have to do something additionally to also have DeletionProtectionEnabled be available on the L1 resource CfnLogGroup, but am happy to do so if instructed.
Reason for this change
aws-logs.LogGroup did not yet support the DeletionProtectionEnabled property. This PR adds it.
Description of changes
This is a standard change to support a boolean property that was added in CloudFormation.
I added the property to the interface LogGroupProps, copying the description from the CloudFormation definition. I also extended the initialization of CfnLogGroup in the constructor. Finally, I created a unit test for the new property.
Describe any new or updated permissions being added
No further changes required.
Description of how you validated changes
Added a unit test which ran successfully.
The following are unrelated to my change, but I wanted to have them mentioned in case it points to me setting up something wrongly in my local environment and introducing errors down the line.
Following the commands in CONTRIBUTING.MD:
The command
npx lerna run build --skip-nx-cachehad 16/17 targets succeed, with the one failing being:Likewise,
yarn buildin the packages/aws-cdk-lib folder failed with messages likebut
yarn testcompleted successfullyChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license